Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Copy all features from the features key in the Cargo.toml. #146

Closed
wants to merge 1 commit into from
Closed

Copy all features from the features key in the Cargo.toml. #146

wants to merge 1 commit into from
+8 −4

Conversation

ripdajacker
Copy link

@ripdajacker ripdajacker commented Jan 25, 2022 •
edited
Loading

Hey

This is my understanding of what is needed to fix issue #145

I've changed the way features are read from the source manifest.
Instead of only copying the single feature enable e.g. crate_name/feature_name, the code now adds all values from the given feature key in the source manifest in addition to crate_name/feature_name.

To take the linked issue as an example, the current version creates a Cargo.toml features section:

[features]
foo = ["dependency_a/foo"]

With this addition the test compiles as expected. The Cargo.toml features section now reads:

[features]
foo = ["dependency_a/foo", "dependency_b/foo"]

dtolnay
dtolnay requested changes Jan 25, 2022
View reviewed changes
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I only skimmed the crate layout you posted in #145, but could you shed some light on why this wouldn't be considered a Cargo bug? If the generated Cargo.toml contains features foo = ["dependency_a/foo"], and dependency_a contains features foo = ["dependency_b/foo"], then trybuild executing cargo test --features foo on the generated manifest should be resulting in dependency_b/foo enabled. Have you looked into why or is there an easy way to understand why that isn't happening?

If it's a Cargo bug then I would prefer not to land this workaround and would ask you to follow up in the Cargo repo.

@ripdajacker
Copy link
Author

Digging into it I hadn't considered it could be a cargo bug.

Your assessment of the layout is correct. dependency_a/foo should in fact enable dependency_b/foo. It seems to only break when using dependency_b/foo from within macro generated code, so it could look like a bug in the incremental compilation in cargo.

I'll report it in cargo, if it's intended behavior I'll reopen the PR.

@ripdajacker
Copy link
Author

I reported it as a cargo issue and they said this is intended behavior as of feature resolver 2 that got released with 1.50.
The issue in question is linked here: Cargo #10331.
They link to this: https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

This means that there in fact is a need for a workaround, but I am no longer sure that my simple PR is enough.

As I understand it:

  • The resolver change is specifically for proc_macro crates
  • Pre 1.5 Rust uses the resolver v1.
  • Post 1.5 has an option to override the resolver version with package.resolver in the toml

So I propose a couple of changes:

  1. When the tested crate is not a proc_macro or we are running rustc < 1.5, use the current logic
  2. When we are in a proc_macro and the rustc version is >= 1.5, use some version of the workaround from this PR
  3. When rustc is >= 1.5 and the crate has package.resolver, copy the resolver key

I would love to hear your thoughts on the matter :)

@ripdajacker ripdajacker reopened this Jan 28, 2022
@dtolnay
Copy link
Owner

dtolnay commented Jan 28, 2022

Always propagating package.resolver + the original change from this PR would be sufficient right?

@dtolnay dtolnay mentioned this pull request Jan 30, 2022
Merged
dtolnay
dtolnay reviewed Jan 30, 2022
View reviewed changes
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the package.resolver change in #149. Reviewing the repro in #145, I think this is otherwise behaving correctly as described in #145 (comment), so I'll close the PR. Thanks!

@dtolnay dtolnay closed this Jan 30, 2022
@ripdajacker
Copy link
Author

Thanks for the very fast resolution!

@ripdajacker ripdajacker deleted the transitive_features branch January 31, 2022 10:42
@ripdajacker ripdajacker restored the transitive_features branch January 31, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ripdajacker @dtolnay